-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Added method DateTime::createFromImmutable() [follow-up for #1145] #2484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ext/date/php_date.c
Outdated
PHP_ME(DateTime, __set_state, NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) | ||
PHP_ME(DateTime, __construct, arginfo_date_create, ZEND_ACC_CTOR|ZEND_ACC_PUBLIC) | ||
PHP_ME(DateTime, __wakeup, NULL, ZEND_ACC_PUBLIC) | ||
PHP_ME(DateTime, __set_state, NULL, ZEND_ACC_PUBLIC|ZEND_ACC_STATIC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't change white space with functional changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was just following the alignment, but ok, reverted.
@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) { | |||
string(8) "DateTime" | |||
} | |||
..and get names of all its methods | |||
array(18) { | |||
array(19) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of this test? (I realise, it has nothing to do with the PR). I'd say we remove it as it just tests that Reflection works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was asking the same question myself, but just cowardly changed the expectf, because I didn't want to remove tests in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is extremely important, since reflection of internal classes is a constant SNAFU
ext/date/php_date.c
Outdated
} | ||
if (old_obj->time->tz_info) { | ||
new_obj->time->tz_info = old_obj->time->tz_info; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines (2842-2849) should be replaced by using timelib_time_clone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also changed createFromMutable to do so (which I simply duplicated).
02970e8
to
810d40a
Compare
@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes. |
Travis failure seems unrelated:
|
I think that boilerplate is, in itself, a good reason to add this. It's an unfortunately not-uncommon thing to need to do, and I imagine it would be easy to mess up (losing or mangling data) if you do it yourself, as well as being plain inefficient. |
@Majkl578 In my opinion it has very limited use unless it is createFromInterface implemented on both DateTime and DateTimeImmutable. |
@enumag From the point of the interface, it doesn't make much sense. Since the interface doesn't know its implementors (OO principle), it could only accept itself and return itself.
Therefore it would be in no way different from __clone (just static). Besides, it even looks strange. Also, since the implementors can't introduce more specific return types (return types are invariant), they could only return the interface as well (doesn't help static analysis). |
810d40a
to
96476ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derickr Thanks for your review, adressed your comments. What do you think about adding this method now, two years later? I still think it'd be a useful addition for interoperability between mutable & immutable datetimes.
☺️
I don't care enough about this method to be here or not.
But I did add some more comments to the code.
NEWS
Outdated
@@ -51,6 +51,8 @@ PHP NEWS | |||
. Fixed bug #74404 (Wrong reflection on DateTimeZone::getTransitions). | |||
(krakjoe) | |||
. Fixed bug #74080 (add constant for RFC7231 format datetime). (duncan3dc) | |||
. Added DateTime::createFromImmutable() method. | |||
(bugs dot php dot net at majkl578 dot cz, Rican7) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an issue for this, and the issue should be mentioned in the NEWS file, just like the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created one: https://bugs.php.net/bug.php?id=74668
@@ -2816,7 +2822,28 @@ PHP_METHOD(DateTimeImmutable, __construct) | |||
} | |||
/* }}} */ | |||
|
|||
/* {{{ proto DateTimeImmutable::createFromMutable(DateTimeZone object) | |||
/* {{{ proto DateTime::createFromImmutable(DateTimeImmutable object) | |||
Creates new DateTime object from an existing immutable DateTimeImmutable object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not make more sense to group this with the other DateTime methods, instead of just before the DateTimeImmutable/createFromMutable method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where exactly should I put it then? There are __construct for mutable & immutable together, __set_state for both together as well...
@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) { | |||
string(8) "DateTime" | |||
} | |||
..and get names of all its methods | |||
array(18) { | |||
array(19) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@Majkl578 I didn't mean adding the method to the ineterface. I meant this: class DateTime implements DateTimeInterface {
static function createFromInterface(DateTimeInterface $other) : DateTime {...}
}
class DateTimeImmutable implements DateTimeInterface {
static function createFromInterface(DateTimeInterface $other) : DateTimeImmutable {...}
} |
hope we will see the light .. ;( |
I really don't like this (as I don't like the
So the |
96476ce
to
495b260
Compare
If you don't, the call wouldn't be type safe (any static analyser like PHPStan would fail otherwise due to undefined method) and you would end up with instanceof check either DateTime or DateTimeImmutable anyway.
This is correct. Most of the time when you need this sort of methods is a compatibility between legacy and modern code, where you'd be using a concrete type anyway. This is also why the argument and return type are not DateTimeInterface. As I've already said, it doesn't make sense to make a generic method for both DateTime and DateTiumeImmutable in the interface. And even if we did, the method would work in a half-redundant way - it would either transform the other type or duplicate __clone. |
@derickr Is this okay to merge? |
@@ -27,7 +27,7 @@ object(ReflectionClass)#%d (1) { | |||
string(8) "DateTime" | |||
} | |||
..and get names of all its methods | |||
array(18) { | |||
array(19) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is extremely important, since reflection of internal classes is a constant SNAFU
@@ -53,6 +53,7 @@ PHP_FUNCTION(getdate); | |||
PHP_METHOD(DateTime, __construct); | |||
PHP_METHOD(DateTime, __wakeup); | |||
PHP_METHOD(DateTime, __set_state); | |||
PHP_METHOD(DateTime, createFromImmutable); | |||
PHP_FUNCTION(date_create); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say fromImmutable
, since you aren't really creating it, but rather casting it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but there is already DateTimeImmutable::createFromMutable(), so the naming is chosen mostly for consistency. :/
ping for 7.2 |
@Majkl578 the window for 7.2 is already closed, so this can sadly only land in 7.3 |
7.2 isn't in RC phase afaik, is it? Should be fine then. |
Merged as d9d13ab into master. Feature freeze is at beta, not RC. However, this kind of addition can still go in under the "small self-contained feature addition" clause, if the RMs approve. @remicollet @sgolemon What do you think about landing this in 7.2? |
@nikic: Thanks! 🎉 Last try before giving up - @remicollet @sgolemon would this be eligible for freeze exception as a small self-contained addition? Thanks. |
Oh wow. I'm just now seeing this. I want to thank @Majkl578 for resurrecting this, @derickr and @Ocramius for reviewing this, and to @nikic for merging this. I also really appreciate the contribution credit received and noted in the Honestly, I was quite disheartened with the PHP contribution process nearly 3 years ago when I had originally created the #1145 PR. It was my first attempt at a code contribution to the PHP source, and I made quite an effort to make the contribution as clean and clear as possible, which was certainly a challenge with my incredibly limited C experience. Ultimately, the hasty actions taken by @smalyshev (2d9399a) with incredibly little communication based on a single message on the internals mailing list, was a really frustrating and unnecessary experience, as is now apparent by the merging of this recreated work. Ultimately, I'm happy to see the code being merged in, and I appreciate the contribution credit, however I wish to make a plea with the internals team to reflect on why this user-friendly and simple change was now considered and merged in nearly 3 years and 4 minor releases later. I only make this plea to hope that we can further encourage contribution in the future, rather than seemingly discouraging it. Thank you! 🙂 Additional context: |
This addition has not been documented yet: http://php.net/manual/en/class.datetime.php — please add documentation. |
I agree that the absence of this method blocks consistent transition of legacy projects to using immutable dates. TBH, it's been blocking us for several years now - every attempt has been reverted. However, the method really needed to be in PHP 5.5 and not 7.3+.
And most people are one or the other most of the time, so nobody cares. I'd suggest that virtually nobody will benefit from this method for a couple of years. That doesn't mean it shouldn't happen. |
It's available in PHP 7.3, see [FR 74668](https://bugs.php.net/bug.php?id=74668) and php/php-src#2484
It's available in PHP 7.3, see [FR 74668](https://bugs.php.net/bug.php?id=74668) and php/php-src#2484
Hi, it seems like this method is missing from the documentation: https://www.php.net/manual/en/class.datetime.php |
FTR, it has been documented in the meantime. |
This basically resurrects PR #1145 which was initially merged, but then reverted because of some not really relevant objections.
The primary purpose of this method should be interoperability between DateTime and DateTimeImmutable. The further already has createFromMutable method, but the former has none. So if one wants to convert mutable to immutable, it's as easy as:
but to convert immutable to mutable, it requires annoying boilerplate:
There are still tons of libraries that rely only on DateTime class (looking at you, Doctrine). Of course, any new code should use the immutable variant, but it's not always feasible as such libraries wouldn't understand the immutable type - therefore it requires conversion.
I still believe it was a wrong decision to revert this before PHP 7, especially because the only arguments against were totally off topic / invalid.
Quoting @derickr's response from the mailing list, which was the only objection as far as I know:
These arguments are totally irrelevant. This is not a factory method, it's a method for interoperability and compatibility. Pretty much like it'd be
$mutable->toImmutable()
or$immutable->toMutable()
.Also the hack is totally meaningless and kind of funny, since the libraries that use DateTime are obviously doing it for purpose (i.e. backwards compatibility - semver anyone?).
I'd really like to see this resolved befrore PHP 7.2. It has already been blocked for two years and @derickr didn't even bother to reply to objections on the original PR...